app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2Widget_selectedRow_Input_widget_spec.js (2)

Line range hint 14-26: Class, let's improve our test reliability by avoiding cy.wait.

In our first test case, we're using cy.wait("@updateLayout"). While this works, it's like watching the clock instead of checking if your experiment is actually complete. Let's consider using Cypress's built-in retry-ability for a more robust approach. For example:

cy.get('@updateLayout').its('response.body.responseMeta.status').should('eq', 200);

This way, we're actually checking for the condition we care about, rather than just waiting for a fixed amount of time.

---

Line range hint 14-45: Let's make our test descriptions more informative, students!

Our current test case descriptions are a bit like vague essay titles. They tell us what we're testing, but not the specific scenario or expected outcome. Let's make them more descriptive. For example:

1. "Input widget displays default value from table widget v2 correctly"
2. "Input widget updates correctly when a different row is selected in the table"

Remember, good test descriptions are like good essay titles - they should give a clear idea of what's being tested and what the expected outcome is.

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_DefaultSearch_Input_spec.js (2)

Line range hint 41-43: Let's clean up our work, class!

The cy.log() statement here is like writing notes in the margin of your test book - it's helpful while you're working, but not necessary for the final product. We can remove this line to keep our test clean and focused:

cy.readTableV2dataPublish("0", "0").then((tabData) => {
  const tabValue = tabData;
  expect(tabValue).to.be.equal("2736212");
  // Remove the following line
  // cy.log("the value is" + tabValue);
  cy.get(publish.inputWidget + " " + "input")
    .first()
    .invoke("attr", "value")
    .should("contain", tabValue);
});

---

Line range hint 43-47: Let's make our test more robust, like a well-built science project!

Instead of using complex selectors, we should use data-* attributes. This makes our tests more resilient to changes in the UI. Here's how we could improve this part:

cy.get('[data-cy="input-widget"]')
  .first()
  .invoke("attr", "value")
  .should("contain", tabValue);

Remember to add the corresponding data-cy attribute to your input widget in the actual application code.

app/client/cypress/limited-tests.txt (1)

Line range hint 26-29: Let's improve our documentation, shall we?

Class, I appreciate the inclusion of instructions for running all specs and the note about ci-test-limit. However, to make this file even more user-friendly, consider the following suggestions:

1. Add a brief explanation of when and why one might want to run all specs.
2. Provide more context about the ci-test-limit command. What does it do, and when should it be used?
3. Consider adding a section with examples of how to use this file in different scenarios.

Remember, clear documentation is like a good lesson plan – it helps everyone understand and follow along more easily!

Here's a suggested improvement:

 # For running all specs - uncomment below:
 #cypress/e2e/**/**/*
 
-#ci-test-limit uses this file to run minimum of specs. Do not run entire suite with this command.
+# Note: The ci-test-limit command uses this file to run a minimum set of specs.
+# It should not be used to run the entire test suite.
+
+# Usage examples:
+# 1. To run specific tests: Use the lists above
+# 2. To run all tests: Uncomment the line above
+# 3. For CI with limited tests: Use the ci-test-limit command

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_Widget_API_Pagination_spec.js (2)

Line range hint 33-39: Well done, class! Let's review the changes.

I'm pleased to see the expansion of the pagination section and the verification of pagination functionality. However, we can make a small improvement to follow best practices.

Let's replace the static wait with a more robust assertion:

-      agHelper.WaitUntilToastDisappear("done");
+      cy.get(commonlocators.toastMsg).should('not.exist');

This change will make our test more reliable and less prone to timing issues. Remember, in testing, we always want to avoid unnecessary waits!

---

Line range hint 41-58: Excellent work on addressing Bug #22477!

Your test case effectively verifies the behavior of the next page button when the last page is reached. I'm particularly impressed with your attention to detail in checking both the disabled state and the absence of a toast message.

To make our test even more robust, let's add an assertion to verify that we're on the last page:

agHelper.GetNClick(table._nextPage("v2"));

agHelper.AssertAttribute(table._nextPage("v2"), "disabled", "disabled");
agHelper.AssertElementAbsence(locators._toastMsg);
// Add this line
cy.get('.t--widget-tablewidgetv2 .page-item').last().should('contain', '2');

This additional assertion will ensure that we're truly on the last page when the next button is disabled. Remember, in testing, we can never be too thorough!

app/client/cypress/e2e/Regression/ClientSide/Binding/InputWidget_TableV2_Sorting_spec.js (2)

36-36: Excellent addition to our test, students!

You've shown great attention to detail by expanding the row selection section before proceeding with the test. This ensures our test won't fail simply because a section is collapsed. Gold star for you!

However, let's make it even better. Can anyone tell me how we might improve this further? That's right! We should add a small wait after expanding the section to ensure it's fully loaded. Let's modify it like this:

table.ExpandIfCollapsedSection("rowselection");
cy.wait(500); // Short wait to ensure section is fully expanded

This will make our test even more robust. Remember, in testing as in life, patience is a virtue!

---

Line range hint 1-78: Class, let's review our work as a whole.

You've done a commendable job creating tests for our input and table widget interactions. Your coverage of default values, sorting, and column identification shows good thinking. However, we have some areas for improvement:

1. I notice we're using cy.wait() in several places. Remember our lesson on avoiding explicit waits? Instead, let's use Cypress's built-in retry-ability. For example, replace:

   cy.wait(1000);
   cy.readTableV2dataPublish("0", "0")

   with:

   cy.readTableV2dataPublish("0", "0", { timeout: 10000 })

2. We're using attribute selectors like publish.inputWidget + " " + "input". Let's improve this by using data attributes. Modify your app code to include data attributes, then update the selectors:

   cy.get('[data-cy="input-widget"]')

3. Lastly, let's enhance our assertions. Instead of separate expect and cy.log statements, combine them:

   cy.readTableV2dataPublish("0", "0").should((tabValue) => {
     expect(tabValue).to.equal("6788734");
     cy.get('[data-cy="input-widget"]').should('have.value', tabValue);
   });

By making these changes, we'll have more robust and maintainable tests. Remember, good testing is the foundation of reliable software!

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/Edge_case_spec.js (2)

Line range hint 70-70: Let's remove this wait, shall we?

Class, I've spotted a cy.wait(1000) on line 70. Remember our golden rule: avoid using cy.wait in our Cypress tests. It's like watching the clock during an exam - it doesn't help and only makes time pass slower!

Instead, let's think about what we're waiting for. Are we waiting for an element to appear? A network request to complete? Let's use Cypress's built-in retry-ability and wait for the specific condition we need.

For example, if we're waiting for rows to be selected, we could do something like:

cy.get('.selected-row').should('have.length', 3)

This way, we're telling Cypress exactly what to expect, rather than arbitrarily waiting. It's like raising your hand when you're done with a question, instead of just waiting for a set amount of time!

---

Line range hint 33-35: Time to refine our selectors, class!

I've noticed we're using CSS selectors in our cy.get() calls. While these work, they're not the best practice according to our coding guidelines. It's like using a nickname instead of a student's proper name - it might work, but it's not always clear or consistent!

Let's improve these by using data-* attributes instead. For example, instead of:

cy.get(`${widgetsPage.textWidget} .bp3-ui-text`)

We could use something like:

cy.get('[data-testid="text-widget-content"]')

This makes our tests more resilient to changes in the CSS and clearer to read. It's like using proper nouns in your essays - much more precise!

Remember to update the application code to include these data-* attributes where needed. It's a bit more work now, but it'll make our tests much easier to maintain in the future!

Also applies to: 44-46, 54-56, 65-67, 75-77

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_MultiRowSelect_spec.js (2)

Line range hint 41-53: Impressive work on this test case, students!

Your test demonstrates a thorough understanding of event handling and user feedback verification. The inclusion of toast message checks is particularly commendable.

However, I have a small suggestion to enhance your test:

Consider adding a negative test case to ensure that the toast message doesn't appear when a row is deselected. This would provide more comprehensive coverage of the onRowSelected behavior.

Here's an example of how you might implement this:

// Deselect the first row
cy.get(".t--table-multiselect").first().click({ force: true });
// Ensure the toast message doesn't appear
cy.get(commonlocators.toastmsg).should("not.exist");

---

Line range hint 55-73: Excellent work on this final test case, class!

Your test thoroughly covers the behavior of default selected rows and their interaction with the header cell. You've maintained our best practices throughout, which is commendable.

To further enhance this test, I have a small suggestion:

Consider adding an assertion to verify the state of the header cell after selecting and deselecting all rows. This would ensure that the header cell's state is consistent with the row selection state.

Here's an example of how you might implement this:

// After selecting all rows
cy.get(".t--table-multiselect-header").first().should("have.class", "selected");

// After deselecting all rows
cy.get(".t--table-multiselect-header").first().should("not.have.class", "selected");

app/client/cypress/e2e/Regression/ClientSide/Binding/TextTableV2_spec.js (1)

Line range hint 116-137: Well done, class! This new test case is a valuable addition.

The new test case for verifying default row selection functionality is well-structured and comprehensive. It covers both the editor and deployed states of the application, which is excellent practice.

However, let's make a small improvement to enhance our test's reliability:

Consider replacing the hard-coded wait with a more robust solution:

-cy.wait("@updateLayout");
+cy.intercept("POST", "/api/v1/layouts/update").as("updateLayout");
+cy.wait("@updateLayout").its("response.statusCode").should("equal", 200);

This change will make our test more resilient to timing issues and ensure that the layout update has completed successfully before proceeding.

app/client/src/widgets/MultiSelectWidgetV2/widget/index.tsx (2)

Line range hint 1-24: Excellent work on improving the componentDidUpdate method, students!

You've made some smart changes here:

1. Adding a check for string or finite values in defaultOptionValue improves our type handling.
2. Using xorWith and equal for change detection is a more robust approach.

These changes will make our component more reliable and easier to maintain. However, I have a small suggestion to make the code even clearer:

Consider extracting the change detection logic into a separate helper function. This would make the componentDidUpdate method easier to read and test. Here's an example:

private hasDefaultOptionValueChanged(
  currentValue: string[] | OptionValue[],
  prevValue: string[] | OptionValue[]
): boolean {
  const isStringArray = currentValue.some(
    (value: any) => isString(value) || isFinite(value)
  );

  return isStringArray
    ? xorWith(currentValue as string[], prevValue as string[], equal).length > 0
    : xorWith(currentValue as OptionValue[], prevValue as OptionValue[], equal).length > 0;
}

Then in componentDidUpdate, you could simply call:

if (this.hasDefaultOptionValueChanged(this.props.defaultOptionValue, prevProps.defaultOptionValue) && this.props.isDirty) {
  this.props.updateWidgetMetaProperty("isDirty", false);
}

This would make your code more modular and easier to understand at a glance. Keep up the good work!

---

Line range hint 1-13: A gold star for adding the setSelectedOptions setter!

This is a valuable addition to our MultiSelectWidget. It allows other parts of the application to programmatically set the selected options, which is very useful for creating dynamic user interfaces.

Great job on enhancing the widget's functionality!

To make sure everyone understands how to use this new setter, let's add some documentation. Consider adding a comment above the setSelectedOptions configuration like this:

/**
 * Sets the selected options of the MultiSelect widget.
 * @param {array} options - An array of option values to be set as selected.
 */
setSelectedOptions: {
  path: "defaultOptionValue",
  type: "array",
  accessor: "selectedOptionValues",
},

This will help other developers understand how to use this setter effectively. Remember, clear documentation is like a good lesson plan - it helps everyone learn and use the code correctly!

app/client/src/widgets/SelectWidget/widget/index.tsx (2)

Line range hint 165-171: Excellent simplification of our property updates!

Students, observe how we've streamlined our getPropertyUpdatesForQueryBinding method. By removing the conditional check and always updating the dynamicPropertyPathList with 'sourceData', we've made our code more consistent and easier to understand.

However, let's consider a small improvement in our code style:

Consider using the spread operator for clarity:

-dynamicPropertyPathList.push({ key: "sourceData" });
+dynamicPropertyPathList = [...dynamicPropertyPathList, { key: "sourceData" }];

This approach creates a new array, which can be beneficial in certain React contexts and makes the addition of the new item more explicit.

---

Line range hint 1012-1023: Bravo on optimizing our filter change handling!

Class, let's examine the improvements in our onFilterChange method. We're now updating the widget's meta property 'filterText' and intelligently triggering server-side filtering only when necessary. This is an excellent optimization that will improve our application's performance.

To further enhance this method, consider the following suggestion:

We could add a debounce mechanism to prevent rapid successive calls to onFilterUpdate. This would be particularly beneficial for performance in cases of fast typing. Here's how you might implement it:

import { debounce } from 'lodash';

// At the class level
private debouncedFilterUpdate = debounce((value: string) => {
  if (this.props.onFilterUpdate && this.props.serverSideFiltering) {
    super.executeAction({
      triggerPropertyName: "onFilterUpdate",
      dynamicString: this.props.onFilterUpdate,
      event: {
        type: EventType.ON_FILTER_UPDATE,
      },
    });
  }
}, 300); // 300ms delay

// In the onFilterChange method
onFilterChange = (value: string) => {
  this.props.updateWidgetMetaProperty("filterText", value);
  this.debouncedFilterUpdate(value);
};

This change would make our widget even more efficient and responsive. What do you think about this enhancement?

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2_ClientSide_Search_spec.js (1)

Line range hint 29-29: Let's avoid using cy.wait; use assertions or built-in retries instead

To make our tests more efficient and less flaky, we should avoid using cy.wait. Instead, we can use assertions or Cypress's built-in retry mechanisms to wait for elements or conditions.

You can replace the cy.wait(1000); with an assertion that checks for the desired state:

-// Disable Client Search
-_.agHelper.CheckUncheck(commonlocators.clientSideSearch, false);
-cy.wait(1000); //wait & then read the table value
+// Disable Client Search and wait for the table to update
+_.agHelper.CheckUncheck(commonlocators.clientSideSearch, false);
+cy.readTableV2dataPublish("0", "0").should("exist");

This way, the test waits dynamically for the table to update without a fixed delay.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/columnTypes/switchCell_spec.js (4)

Line range hint 130-131: Avoid Using cy.wait Statements

It's important to understand that using cy.wait with static wait times can lead to flaky tests. Instead, you should use assertions or commands that wait for certain conditions to be met, ensuring your tests are reliable and stable.

Please remove the cy.wait(100); statement and rely on an assertion to verify that the necessary conditions are met before proceeding. Here's how you can adjust your code:

- cy.wait(100);
  agHelper.ValidateToastMessage("This is a test message");

The ValidateToastMessage function should handle the waiting internally by checking for the toast message's appearance.

---

Line range hint 76-77: Use Data Attributes Instead of CSS Selectors

When selecting elements for interaction, it's best to use data-* attributes. This practice enhances the robustness of your tests by reducing their susceptibility to UI changes that might affect class names or structures.

Please update your selectors to use data-* attributes or locator variables. For example:

- cy.get(".t--propertypane").contains("Style").click({ force: true });
+ cy.get("[data-testid='t--propertypane']").contains("Style").click({ force: true });

This change will make your test more resilient and easier to maintain.

---

Line range hint 78-80: Avoid Complex CSS Selectors; Utilize Locator Variables

Using long or complex CSS selectors can make your tests fragile and harder to read. It's advisable to use locator variables, which are predefined selectors that improve code readability and maintainability.

Please modify your code to use locator variables from your locators file. For example:

- cy.get(".t--property-control-horizontalalignment .ads-v2-segmented-control__segments-container")
+ cy.get(commonLocators.horizontalAlignmentSegment)

This adjustment will help ensure that your tests are less affected by changes in the UI structure.

---

Line range hint 125-126: Enhance Test Reliability Without Static Waits

Using static waits like cy.wait(100); doesn't guarantee that the application is ready for the next action, which can cause intermittent test failures.

Instead of a static wait, consider using an assertion to confirm that the expected element or message is present before proceeding. For example:

- cy.wait(100);
  agHelper.ValidateToastMessage("This is a test message");
+ agHelper.ValidateToastMessage("This is a test message");

The ValidateToastMessage function should inherently wait for the toast message to appear, so the explicit wait is unnecessary.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_1_Spec.ts (1)

Line range hint 38-43: Avoid hardcoded strings in assertions

To make your tests more robust and easier to maintain, it's advisable to avoid hardcoding strings in your assertions. Instead, define expected values as constants or variables. This approach improves clarity and simplifies updates.

Consider applying this change:

+const expectedUserName = "Lindsay Ferguson";
table.ReadTableRowColumnData(1, 3, "v2").then((cellData) => {
-  expect(cellData).to.eq("Lindsay Ferguson");
+  expect(cellData).to.eq(expectedUserName);

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/AddNewRow1_spec.js (12)

Line range hint 21-28: Avoid Using Plain Strings for Selectors

Using class selectors like .t--property-control-allowaddingarow is discouraged. Instead, utilize data attributes and locator variables for consistency and reliability.

Refactor the selectors as follows:

-cy.get(".t--property-control-allowaddingarow").should("exist");
-cy.get(".t--property-control-allowaddingarow input").should("exist");
-cy.get(".t--add-new-row").should("not.exist");
-propPane.TogglePropertyState("Allow adding a row", "Off", null);
-cy.get(".t--add-new-row").should("not.exist");
-cy.get(".t--property-control-onsave").should("not.exist");
-cy.get(".t--property-control-ondiscard").should("not.exist");
-cy.get(".t--property-control-defaultvalues").should("not.exist");
+cy.get(propPane._allowAddARow).should("exist");
+cy.get(propPane._allowAddARowToggle).should("exist");
+cy.get(table._addNewRow).should("not.exist");
+propPane.TogglePropertyState("Allow adding a row", "Off");
+cy.get(table._addNewRow).should("not.exist");
+cy.get(propPane._onSave).should("not.exist");
+cy.get(propPane._onDiscard).should("not.exist");
+cy.get(propPane._defaultValues).should("not.exist");

Ensure that the locator variables like propPane._allowAddARow and table._addNewRow are defined in your locators file.

---

Line range hint 34-39: Avoid Disabling Tests Temporarily with Code

Instead of disabling the add new row link by entering inline edit mode, consider programmatically setting the state or mocking the behavior. This avoids potential flakiness in tests.

---

Line range hint 47-49: Use Data Attributes for Selectors

Selecting elements using class names can be unreliable. Replace:

cy.get(".tableWrap .new-row").should("exist");

With a selector that uses data attributes or locator variables.

Refactor as:

-cy.get(".tableWrap .new-row").should("exist");
+cy.get(table._newRow).should("exist");

Define table._newRow in your locators file.

---

Line range hint 54-56: Use Locator Variables Instead of Plain Strings

In the line:

cy.updateCodeInput(
  ".t--property-control-defaultvalues",
  "{{{step: 'newStepCell'}}}",
);

Replace the plain string selector with a locator variable.

Refactor as:

-cy.updateCodeInput(
-  ".t--property-control-defaultvalues",
-  "{{{step: 'newStepCell'}}}",
-);
+cy.updateCodeInput(
+  propPane._defaultValues,
+  "{{{step: 'newStepCell'}}}",
+);

Ensure propPane._defaultValues is defined.

---

Line range hint 62-65: Avoid Using Plain Strings in Assertions

When asserting table data, use locator variables and avoid hardcoded indices.

Refactor as:

-cy.readTableV2data(0, 0).then((val) => {
+cy.readTableV2data(rowIndex, colIndex).then((val) => {
   expect(val).to.equal("newStepCell");
 });

Define rowIndex and colIndex as needed.

---

Line range hint 71-88: Encapsulate Repeated Selectors Using Locator Variables

The repeated use of selectors like .t--widget-tablewidgetv2 .t--search-input should be replaced with locator variables to improve test maintainability.

Refactor as:

-cy.get(".t--widget-tablewidgetv2 .t--search-input").should("exist");
+cy.get(table._searchInput).should("exist");

Repeat this change for all similar selectors, defining appropriate locator variables in your locators file.

---

Line range hint 92-109: Avoid Using cy.get with Complex Selectors

Using complex CSS selectors can make tests brittle.

Refactor selectors to use data attributes or locator variables. For example:

-cy.get(".t--discard-new-row").click({ force: true });
+cy.get(table._discardNewRow).click();

Define table._discardNewRow accordingly.

---

Line range hint 115-124: Use Locator Variables for Dynamic Selectors

When constructing selectors with variables, ensure you're using locator variables and data attributes.

Refactor:

-cy.get(
-  `[data-colindex=0][data-rowindex=0] .t--inlined-cell-editor`,
-).should("exist");
+cy.get(table._inlinedCellEditor(0, 0)).should("exist");

Define table._inlinedCellEditor as a function that returns the selector string.

---

Line range hint 129-136: Avoid Hardcoding Values in Tests

Instead of hardcoding indices and values, use constants or variables for better readability and flexibility.

Consider defining variables for row and column indices, and expected values.

---

Line range hint 142-146: Use Proper Widget Selection Methods

When adding widgets, use the appropriate helper functions to interact with the canvas, ensuring consistency across tests.

Refactor:

-cy.dragAndDropToCanvas("textwidget", { x: 300, y: 600 });
+entityExplorer.DragDropWidgetNVerify("textwidget", 300, 600);

Ensure entityExplorer is correctly imported and used.

---

Line range hint 161-168: Use Exact Property Labels for Toggling

When toggling properties, ensure the labels match exactly to avoid errors due to typos or formatting differences.

Verify that the property labels used in propPane.TogglePropertyState match those in the UI.

---

Line range hint 172-174: Avoid Using cy.wait in Tests

The explicit wait cy.wait(1000); should be replaced with proper synchronization.

Refactor as:

-cy.wait(1000);
+agHelper.WaitUntilToastDisappear();

Or wait for a specific element to appear or disappear.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter2_2_Spec.ts (5)

Line range hint 141-141: Avoid using agHelper.Sleep() to ensure test reliability

Using agHelper.Sleep() can make our tests less reliable because it introduces arbitrary delays. Instead, let's wait for specific elements or conditions to ensure our application is ready before proceeding.

---

Line range hint 165-165: Avoid using agHelper.Sleep() to enhance test efficiency

Including agHelper.Sleep() can lead to inefficient and flaky tests. Let's use explicit waits or assertions to synchronize our test execution with the application's state.

---

Line range hint 182-183: Refrain from using cy.wait() with fixed timeouts

Relying on cy.wait(500) can cause our tests to be flaky. It's better to wait for specific elements or events. Let's find a more reliable synchronization method to improve test stability.

---

Line range hint 183-184: Use data-* attributes and locator variables for selectors

In lines 183-184, using complex CSS selectors like ".t--table-filter:nth-child(2) .t--table-filter-value-input input[type=text]" can make our tests brittle. Let's utilize data-* attributes and locator variables to enhance the robustness and maintainability of our tests.

---

Line range hint 240-240: Avoid using fixed timeouts like .wait(500)

Using .wait(500) after typing can make tests flaky. Instead, consider waiting for specific elements or conditions to ensure the application is ready before proceeding.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_spec.js (2)

Line range hint 69-70: Use Data- Attributes Instead of CSS Selectors*

When selecting elements in your tests, it's important to use data-* attributes rather than CSS selectors. Using selectors like cy.contains('[role="columnheader"] .draggable-header', "userName") can make your tests brittle, as any change in the DOM structure or classes can break them. By using data-* attributes, you ensure that your tests are more stable and maintainable.

Consider updating your code as follows:

- cy.contains('[role="columnheader"] .draggable-header', "userName")
+ cy.get("[data-colindex='3'] .draggable-header").click({ force: true });

Ensure that you have the appropriate data-* attributes added to your elements in the application code to support this change.

---

Line range hint 71-71: Avoid Using Fixed Delays Like cy.wait(1000);

Relying on fixed delays such as cy.wait(1000); can lead to unreliable tests. The application might take more or less time to reach the desired state, causing tests to fail intermittently. Instead, it's better to wait for a specific condition or element to appear before proceeding.

Here's how you can improve your test:

- cy.wait(1000);
+ cy.get(".t--table-widget").should("be.visible");

Replace the selector with the one that best indicates the table has finished sorting.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2Filter1_2_Spec.ts (4)

Line range hint 41-41: Replace arbitrary waits with condition-based waits

At line 41:

agHelper.GetNClick(table._filterInputValue, 0).type("19").wait(500);

Using .wait(500); introduces an unnecessary delay and can cause the test to be unreliable. It's better to wait for the expected change in the table data.

You can remove the .wait(500); and add an assertion or wait for a specific element to confirm the filter has applied:

agHelper.GetNClick(table._filterInputValue, 0).type("19");
table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Tobias Funke");
});

---

Line range hint 96-96: Avoid using .wait(500);—wait for the expected condition instead

In line 96, after modifying the filter input:

.wait(500);

This fixed wait might not align with the actual time needed for the table to update.

Replace .wait(500); with a check for the updated table data:

table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Ryan Holmes");
});

---

Line range hint 147-147: Use dynamic waits rather than fixed delays

At line 147:

.wait(500);

Fixed waits can make tests slower and less reliable. It's better to wait for the action to complete by checking for a specific outcome.

After typing in the new filter value, add an assertion to wait for the table to reflect the changes:

table.ReadTableRowColumnData(0, 3, "v2").then(($cellData) => {
  expect($cellData).to.eq("Ryan Holmes");
});

---

Line range hint 26-26: Remember to avoid using agHelper.Sleep(); in tests

At line 26:

agHelper.Sleep(2000); //table to filter & records to disappear

Again, using fixed sleep periods isn't recommended. We should wait for a specific condition instead.

Use a method like table.WaitForTableEmpty("v2"); or check for the expected data.

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_DisplayText_spec.ts (1)

Line range hint 43-43: Avoid using agHelper.Sleep(1000) to improve test reliability.

On line 43, the use of agHelper.Sleep(1000) is discouraged. Relying on static waits can lead to flaky tests due to timing inconsistencies. Instead, consider using dynamic waits that respond to application state changes.

Replace the sleep with a proper wait method, such as waiting for an element to be visible or a network call to complete. This ensures your test proceeds only when the application is ready, enhancing stability.

Configuration used: .coderabbit.yaml
Review profile: CHILL

app/client/cypress/e2e/Regression/ClientSide/DynamicHeight/JsonForm_spec.ts (1)

Line range hint 1-65: Class, let's talk about good habits in our code!

Now, children, I've noticed something in our test that we need to improve. Can anyone spot the line that appears several times and starts with agHelper.Sleep? Yes, that's right!

Using agHelper.Sleep(5000) is like taking a nap in the middle of a test - it's not very efficient! Instead of sleeping, we should be actively waiting for something to happen. Here are some better ways to do this:

1. Use cy.wait('@apiCall') if you're waiting for a specific API call to complete.
2. Use cy.get('element').should('be.visible') to wait for an element to appear.
3. If you're waiting for a change in an element's property, use something like cy.get('element').should('have.css', 'height', expectedHeight).

Remember, class, in testing as in life, it's better to be active and attentive rather than just waiting around!

Let's work on replacing these agHelper.Sleep(5000) calls with more appropriate waiting mechanisms. Who wants to come to the board and show us how to do the first one?

app/client/cypress/e2e/Regression/ClientSide/Binding/TableV2TextPagination_spec.js (3)

Line range hint 96-97: Consider using data- attributes or locator variables instead of class selectors*

Using class selectors like ".show-page-items" and ".page-item" can make your tests fragile if class names change in the future. It's a good practice to use data-* attributes or predefined locator variables for more robust and maintainable selectors.

You might update the selectors like this:

- cy.get(".show-page-items").should("contain", "20 Records");
+ cy.get(locators._showPageItems).should("contain", "20 Records");

- cy.get(".page-item").next().should("contain", "of 2");
+ cy.get(locators._pageItem).next().should("contain", "of 2");

Ensure that the appropriate locator variables locators._showPageItems and locators._pageItem are defined in your locators file.

---

Line range hint 99-99: Avoid using class selectors; prefer data- attributes or locator variables*

Referencing elements by class names like ".t--table-widget-next-page" may lead to brittle tests if those class names change. It's advisable to use data-* attributes or locator variables for more stable and maintainable selectors.

Consider updating your selectors:

- cy.get(".t--table-widget-next-page").should("not.have.attr", "disabled");
+ cy.get(locators._tableWidgetNextPage).should("not.have.attr", "disabled");

...

- cy.get(".t--table-widget-next-page").should("have.attr", "disabled");
+ cy.get(locators._tableWidgetNextPage).should("have.attr", "disabled");

Make sure that locators._tableWidgetNextPage is properly defined in your locators file.

Also applies to: 103-103

---

Line range hint 102-102: Avoid using static waits like cy.wait(500)

Including fixed-duration waits can make your tests less reliable and slower. It's better to wait for specific conditions or elements to ensure that the application is ready before proceeding.

Consider replacing the static wait with a dynamic wait. Since you're checking that the "Next Page" button has the disabled attribute in the next line, you might not need the static wait at all.

- cy.wait(500);

If a wait is necessary, you can wait for a network call or an element state:

- cy.wait(500);
+ cy.wait("@postExecute");

Or ensure the element is in the expected state:

- cy.wait(500);
+ cy.get(locators._tableWidgetNextPage).should("have.attr", "disabled");

app/client/cypress/e2e/Regression/ClientSide/Widgets/TableV2/TableV2_PropertyPane_2_spec.js (4)

Line range hint 16-17: Avoid Fixed Delays with cy.wait(5000);

In lines 16-17, you're using cy.wait(5000); to introduce a fixed delay in your test. Relying on fixed delays can lead to flaky tests because it doesn't guarantee that the desired state has been reached. Instead, you should use dynamic waits that wait for specific events or elements.

You already have cy.wait("@updateLayout"); immediately after the fixed wait. This dynamic wait should be sufficient. Let's remove the unnecessary fixed delay:

- // eslint-disable-next-line cypress/no-unnecessary-waiting
- cy.wait(5000);

---

65-65: Use Locator Variables Instead of Plain Strings

In line 65, you're using a plain string "search\\&filters" in the table.ExpandIfCollapsedSection method. According to our coding guidelines, it's better to use locator variables or data-* attributes for locators rather than plain strings. This practice enhances maintainability and reduces the likelihood of errors when locators change.

Consider defining a locator variable for "search\\&filters" in your locators file and using that variable here.

---

Line range hint 70-70: Correct Typographical Errors in Comments

The comment on line 70 contains typographical errors: "// Chage deat search text value to \"data\"". Clear and correct comments improve code readability and maintainability.

Please apply the following correction:

- // Chage deat search text value to "data"
+ // Change default search text value to "data"

---

Line range hint 91-94: Prefer Data- Attributes Over CSS Selectors*

In lines 91-94, you are using a CSS selector ".t--property-control-propertyname pre span span" to select an element. While this works, using data-* attributes for selectors is a better practice. It makes your tests more robust and less likely to break with UI changes.

Consider updating the selector to use a data-* attribute:

- cy.get(".t--property-control-propertyname pre span span").should(
+ cy.get("[data-testid='t--property-control-propertyname']").should(
    "have.text",
    "customColumn18",
);

This change will make your test more maintainable and aligned with best practices.

Configuration used: .coderabbit.yaml
Review profile: CHILL

Configuration used: .coderabbit.yaml
Review profile: CHILL